Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the glimmer syntax, because that's what we use. Not handlebars! #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use the glimmer syntax, because that's what we use. Not handlebars! #46

wants to merge 1 commit into from

Conversation

NullVoxPopuli
Copy link

@NullVoxPopuli NullVoxPopuli commented Sep 11, 2021 •
edited
Loading

Update: the below is resolved, but I think this implementation is potentially sub-optimal

Here is an alternate implementation: https://github.com/empress/ember-showdown-prism/pull/51/files

old post

So... this went goofy.

  • try to upgrade ember-prism
  • ember-prism requires ember-auto-import@v2
  • install ember-auto-import@v2
  • eai@v2 requires webpack@v5, install that
  • change a html tag on a code snippet to glimmer to test the hifi highlighting
  • see error about "rest" something, don't look to far into it
  • try upgrading node to v14
  • error persists, pin npm to v7
  • notice there are tons of vulns at each of these steps
  • delete the package-lock.json to get the 982 vulns down to 10
  • error persists. oof -- maybe it's fastboot related?
Error occurred:

- While rendering:
 -top-level
   application
     show
       dynamic-template


There was an error running your app in fastboot. More info about the error: 
TypeError: Cannot read property 'rest' of undefined
   at Object.tokenize (/tmp/broccoli-49127aWU7OtVD0k0b/out-316-broccoli_merge_trees/assets/vendor/prismjs/prism.js:658:1)
   at Object.highlight (/tmp/broccoli-49127aWU7OtVD0k0b/out-316-broccoli_merge_trees/assets/vendor/prismjs/prism.js:628:1)
   at /tmp/broccoli-49127aWU7OtVD0k0b/out-316-broccoli_merge_trees/assets/addon-tree-output/field-guide/initializers/showdown-extensions.js:50:1

@mansona
Copy link
Member

mansona commented Sep 14, 2021

so I've seen this rest issue before 🤔 I think it's when you're using an unsupported language.

Do you maybe need to add a language to this list for it to work: https://github.com/empress/field-guide/blob/master/index.js#L47-L58 ?

@mansona
Copy link
Member

mansona commented Sep 14, 2021

Also it isn't great that the tests are passing when there is clearly an issue 🙃

@NullVoxPopuli
Copy link
Author

yup, I was missing "markup", which I myself added to the ember-prism README 🤦

It also looks like the shodown initializer is undoing the hifi highlighting for all language tags except for glimmer (because it's new).

I made some changes to the Prism languages in app.js...
But... this begs the question, should this be ember-prism's responsibility?
If so, what does implementation of that look like?

Also, for @Turbo87, who has a project where they don't want prismjs-glimmer, how would we handle that?

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review September 14, 2021 15:13
@mansona
Copy link
Member

mansona commented Sep 14, 2021

It now looks like the deploy preview isn't compiling 🤔 I forced npm@7 on the build to rule that out but it's still not working

@Dhaulagiri Dhaulagiri mentioned this pull request Oct 24, 2022
Merged
2 tasks
@elwayman02
Copy link
Contributor

This should be superseded by #82

@elwayman02
Copy link
Contributor

@NullVoxPopuli can you take another look at this, rebase and see what changes from this PR are still needed? We were able to bump the version successfully, but it looks like you have a few other changes here that haven't landed in other PRs. So, I want to make sure we get them in if they're needed.

@NullVoxPopuli
Copy link
Author

I don't have the time / energy for this atm -- any extra-curricular OSS I do is going to be on limber and related projects, like buttered-ember, and glimdown.
I don't even know if I have anything that uses field-guide 😅

@elwayman02 elwayman02 added enhancement New feature or request dependencies Pull requests that update a dependency file labels Feb 15, 2023
@netlify
Copy link

netlify bot commented Aug 8, 2023 •
edited
Loading

✅ Deploy Preview for field-guide ready!

Name Link
🔨 Latest commit 5ec0f75
🔍 Latest deploy log https://app.netlify.com/sites/field-guide/deploys/64d238cf3708e300086a2271
😎 Deploy Preview https://deploy-preview-46--field-guide.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NullVoxPopuli NullVoxPopuli changed the title Upgrade ember-prism Use the glimmer syntax, because that's what we use. Not handlebars! Aug 8, 2023
NullVoxPopuli
NullVoxPopuli commented Aug 8, 2023
View reviewed changes
tests/dummy/app/app.js
import Application from '@ember/application';
import Resolver from 'ember-resolver';
import loadInitializers from 'ember-load-initializers';
import config from 'dummy/config/environment';

import { setup } from 'ember-prism';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each consuming app will have to do this, unless there is a field-guide entrypoint we can have folks import and do this setup on behalf of people.

NullVoxPopuli
NullVoxPopuli commented Aug 31, 2023
View reviewed changes
tests/dummy/app/app.js
export default class App extends Application {
modulePrefix = config.modulePrefix;
podModulePrefix = config.podModulePrefix;
Resolver = Resolver;
}

loadInitializers(App, config.modulePrefix);
setup();

Prism.languages.hbs = Prism.languages.glimmer;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handlebars is taken care of: https://github.com/shipshapecode/ember-prism/blob/main/addon/index.js#L4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NullVoxPopuli @mansona @elwayman02